Fix unused_parens issue for higher ranked function pointers #106148

chenyukang · 2022-12-26T04:57:31Z

chenyukang · 2022-12-26T04:59:24Z

I think #104397 is caused by the following :,
do you know any better solution for this? @lcnr

compiler/rustc_lint/src/unused.rs

lcnr

do you still want to look into the alternative approach suggested by @cjgillot? I am a bit uncomfortable with using spans for stuff like that as I can never get myself to be certain that we're correctly dealing with macros.

Also, can you add a test for both: (fn()): Trait if we don't have one yet (this should lint), and ((for<'a> fn(&'a u32))): Trait (this should lint for the inner brace).

chenyukang · 2023-01-09T12:35:06Z

do you still want to look into the alternative approach suggested by @cjgillot? I am a bit uncomfortable with using spans for stuff like that as I can never get myself to be certain that we're correctly dealing with macros.

Also, can you add a test for both: (fn()): Trait if we don't have one yet (this should lint), and ((for<'a> fn(&'a u32))): Trait (this should lint for the inner brace).

Yes, I met an issue last time I tried it, will reconsider it later.

lcnr · 2023-01-13T10:12:57Z

compiler/rustc_lint/src/early.rs

+        if let WherePredicate::BoundPredicate(WhereBoundPredicate { bounded_ty, .. }) = p &&
+            let ast::TyKind::BareFn(b) = &bounded_ty.kind &&
+            b.generic_params.len() > 0 {
+                return;


shouldn't this check for ast::TyKInd::Paren containing a BareFn

I think the condition should also include the check bound_generic_params.is_empty().

THis should lint as for<'b> for<'a> fn(Inv<'a>): Trait<'b> is also correct:

#![deny(warnings)] struct Inv<'a>(&'a mut &'a ()); trait Trait<'a> {} impl<'b> Trait<'b> for for<'a> fn(Inv<'a>) {} fn with_bound() where for<'b> (for<'a> fn(Inv<'a>)): Trait<'b>, //~ ERROR unnecessary parentheses around type {} fn main() { with_bound(); }

ah, but instead of changing the visiting behavior for all lints, you should instead add a method like fn enter_lint_where_bound or something to EarlyLintPass where the UnusedParens lint has special behavior. Similar to https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint/trait.EarlyLintPass.html#method.enter_lint_attrs

lcnr · 2023-01-13T10:17:24Z

compiler/rustc_lint/src/unused.rs

@@ -1033,7 +1033,6 @@ impl EarlyLintPass for UnusedParens {
        if let ast::TyKind::Paren(r) = &ty.kind {
            match &r.kind {
                ast::TyKind::TraitObject(..) => {}
-                ast::TyKind::BareFn(b) if b.generic_params.len() > 0 => {}
                ast::TyKind::ImplTrait(_, bounds) if bounds.len() > 1 => {}
                ast::TyKind::Array(_, len) => {
                    self.check_unused_delims_expr(


I just realized, that's a bug 😁 it causes us to not lint on

fn main() { let _x: ([u32; 3]); // ^^^^^^^^^^ should lint let _y: [u8; (3)]; // ^^^ should lint let _z: ([u8; (3)]); // ^^^ does lint }

may make sense as part of this PR. HAve to move the ast::TyKind::Array branch to the same level as the if let ast::TyKind::Paren(r) = &ty.kind {, not inside of TyKind::Paren.

If not I am going to open a separate issue for this.

Seems we find some lint error in std 😂:

error: unnecessary parentheses around const expression --> library/std/src/io/error/repr_bitpacked.rs:377:27 | 377 | static_assert!(@usize_eq: (TAG_MASK & TAG_SIMPLE_MESSAGE), TAG_SIMPLE_MESSAGE); | ^ ^ | = note: `-D unused-parens` implied by `-D warnings` help: remove these parentheses | 377 - static_assert!(@usize_eq: (TAG_MASK & TAG_SIMPLE_MESSAGE), TAG_SIMPLE_MESSAGE); 377 + static_assert!(@usize_eq: TAG_MASK & TAG_SIMPLE_MESSAGE, TAG_SIMPLE_MESSAGE); | error: unnecessary parentheses around const expression --> library/std/src/io/error/repr_bitpacked.rs:378:27 | 378 | static_assert!(@usize_eq: (TAG_MASK & TAG_CUSTOM), TAG_CUSTOM); | ^ ^ | help: remove these parentheses | 378 - static_assert!(@usize_eq: (TAG_MASK & TAG_CUSTOM), TAG_CUSTOM); 378 + static_assert!(@usize_eq: TAG_MASK & TAG_CUSTOM, TAG_CUSTOM); |

…ion pointers

rustbot · 2023-01-14T15:26:10Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

lcnr

thanks 👍

r=me after nits

lcnr · 2023-01-16T13:09:21Z

compiler/rustc_lint/src/unused.rs

@@ -1030,21 +1040,22 @@ impl EarlyLintPass for UnusedParens {
    }

    fn check_ty(&mut self, cx: &EarlyContext<'_>, ty: &ast::Ty) {
+        if let ast::TyKind::Array(_, len) = &ty.kind {


style nit: can you use a match instead of 2 unrelated if let

lcnr · 2023-01-16T13:13:08Z

compiler/rustc_lint/src/unused.rs

+                None,
+                None,
+            );
+        }
        if let ast::TyKind::Paren(r) = &ty.kind {


can you set self.with_self_ty_parens to false at the end of this branch and instead assert that it is false in exit_where_predicate?

Because right now I think you don't lint for the inner paren in (dyn Trait<(for<'a> fn(&'a ())>): Trait, even though we should.

chenyukang · 2023-01-16T16:59:19Z

@rustbot
r=lcnr

lcnr · 2023-01-17T07:18:55Z

@bors r+ rollup

bors · 2023-01-17T07:18:57Z

📌 Commit 9d74bb8 has been approved by lcnr

It is now in the queue for this repository.

Rollup of 5 pull requests Successful merges: - rust-lang#101698 (Constify `TypeId` ordering impls) - rust-lang#106148 (Fix unused_parens issue for higher ranked function pointers) - rust-lang#106922 (Avoid unsafe code in `to_ascii_[lower/upper]case()`) - rust-lang#106951 (Remove ineffective run of SimplifyConstCondition) - rust-lang#106962 (Fix use suggestion span) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rustbot assigned lcnr Dec 26, 2022

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 26, 2022

cjgillot reviewed Dec 26, 2022

View reviewed changes

compiler/rustc_lint/src/unused.rs Outdated Show resolved Hide resolved

lcnr reviewed Jan 9, 2023

View reviewed changes

chenyukang force-pushed the yukang/fix-105061-unused branch 2 times, most recently from ab3b25b to 7689401 Compare January 13, 2023 02:39

lcnr reviewed Jan 13, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

chenyukang added 3 commits January 14, 2023 17:11

fix rust-lang#105061, Fix unused_parens issue for higher ranked funct…

7d99866

…ion pointers

fix issues in unused lint

c67903e

add test case for issue 105601

644ee8d

comments feedback

9d74bb8

lcnr reviewed Jan 16, 2023

View reviewed changes

chenyukang force-pushed the yukang/fix-105061-unused branch from 27934df to 9d74bb8 Compare January 16, 2023 16:09

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 17, 2023

Dylan-DPC mentioned this pull request Jan 17, 2023

Rollup of 5 pull requests #106984

Merged

bors merged commit f91f369 into rust-lang:master Jan 17, 2023

rustbot added this to the 1.68.0 milestone Jan 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix unused_parens issue for higher ranked function pointers #106148

Fix unused_parens issue for higher ranked function pointers #106148

chenyukang commented Dec 26, 2022

chenyukang commented Dec 26, 2022

lcnr left a comment

chenyukang commented Jan 9, 2023

lcnr Jan 13, 2023

lcnr Jan 13, 2023

lcnr Jan 13, 2023

chenyukang Jan 14, 2023

This comment has been minimized.

rustbot commented Jan 14, 2023

lcnr left a comment

lcnr Jan 16, 2023 •

edited

Loading

lcnr Jan 16, 2023

chenyukang commented Jan 16, 2023

lcnr commented Jan 17, 2023

bors commented Jan 17, 2023

Fix unused_parens issue for higher ranked function pointers #106148

Fix unused_parens issue for higher ranked function pointers #106148

Conversation

chenyukang commented Dec 26, 2022

chenyukang commented Dec 26, 2022

lcnr left a comment

Choose a reason for hiding this comment

chenyukang commented Jan 9, 2023

lcnr Jan 13, 2023

Choose a reason for hiding this comment

lcnr Jan 13, 2023

Choose a reason for hiding this comment

lcnr Jan 13, 2023

Choose a reason for hiding this comment

chenyukang Jan 14, 2023

Choose a reason for hiding this comment

This comment has been minimized.

rustbot commented Jan 14, 2023

lcnr left a comment

Choose a reason for hiding this comment

lcnr Jan 16, 2023 • edited Loading

Choose a reason for hiding this comment

lcnr Jan 16, 2023

Choose a reason for hiding this comment

chenyukang commented Jan 16, 2023

lcnr commented Jan 17, 2023

bors commented Jan 17, 2023

lcnr Jan 16, 2023 •

edited

Loading